Fix reflection probes working in stereo on compatibility renderer#102447
Fix reflection probes working in stereo on compatibility renderer#102447BastiaanOlij wants to merge 1 commit into
Conversation
See #97773, which could be amended to initially render reflection probes in a single frame while the scene is loading or being switched for another scene. |
| } | ||
|
|
||
| void CubemapFilter::filter_radiance(GLuint p_source_cubemap, GLuint p_dest_cubemap, GLuint p_dest_framebuffer, int p_source_size, int p_mipmap_count, int p_layer) { | ||
| glDisable(GL_CULL_FACE); |
There was a problem hiding this comment.
| glDisable(GL_CULL_FACE); | |
| glDisable(GL_CULL_FACE); | |
| scene_state.cull_mode = RS::CULL_MODE_DISABLED; |
There was a problem hiding this comment.
@clayjohn I just had a check at this, scene_state is not exposed to any of the effects, and none of the other effects respect the scene state.
We really need to introduce a new object that keeps track of the global OpenGL state that we can access from different areas of compatibility renderer and remove this logic from scene state. But that goes a little beyond the scope of this PR.
There was a problem hiding this comment.
I guess the shortest path forward is to move the change outside of this function and into the calling code. Otherwise it will break the scene shader
There was a problem hiding this comment.
Thing is, we do this stuff everywhere already. I think that is why we keep having issues like these popup at the strangest times. OpenGLs global state sucks :)
clayjohn
added
the
cherrypick:4.4
AThousandShips
added
the
cherrypick:4.5
|
is this gonna be added? this seems like a pretty big issue |
So far I haven't been able to reproduce the issue anymore. Seeing this is global state that is set wrong, the issue appears, or disappears depending on how the previous process leaves the global state. Probably another change in the engine has left the global state such that reflection probes now work. And sadly, its not unlikely as we enhance the engine, that this gets changed again in a way that breaks stuff which is why many effects just set the entire state the way they want, even if its already set correctly, and accept the overhead that brings. The real solution here is the one @clayjohn suggested originally but that is a fairly big rewrite to move, and complete, the code that manages the global state into an object (as it is not accessible from effect/environment code in its current state). Not something we want to do in feature freeze, and something that has a high potential of breaking something unintentionally. I've been wanting to tackle this for some time, just haven't worked up the courage to actually do it. Anyway, long story short is that by the testing I did last time I looked into this, this was working fine. The only issue left is the 6 frame delay for full dynamic reflection probes but that is due to us spreading out the workload over 6 frames. |
|
I just tested this again with Godot 4.6.rc1 and the Compatibility Renderer. Same behavior. The Reflection Probe doesn't work on the Quest (PCVR works). |
Repiteo
removed
cherrypick:4.4
8 participants
When rendering in stereo we do not do our right-side-up rendering of the main buffer. There seems to be a condition where our culling direction remains reversed which stopped the cubemap filter from working and copying the final mipmap results of the reflection probe leaving the contents of the reflection probe empty.
Disabling culling before performing the cubemap filter ensures we always perform the process.
Note that when the reflection probe is set to once, it can take over 6 frames before the reflection probe is populated with data as we render one side of the reflection probe per frame to save performance. So for the first 6 to 15 frames the reflection probe still remains black in the MRP supplied on the bug report causing the pillars to not be lit up correctly.
Fixes #102443